Skip to content

fix(compact): sanitize plaintext compaction replays - #1720

Open
Komzpa wants to merge 7 commits into
Soju06:mainfrom
Komzpa:fix/sanitize-encrypted-compaction-replays
Open

fix(compact): sanitize plaintext compaction replays#1720
Komzpa wants to merge 7 commits into
Soju06:mainfrom
Komzpa:fix/sanitize-encrypted-compaction-replays

Conversation

@Komzpa

@Komzpa Komzpa commented Aug 13, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • strip poisoned local compact fallback messages together with their following compact output item
  • rewrite plaintext replayed compaction summaries into inert assistant text so old local compact summaries do not go back upstream as compaction items
  • preserve provider-looking encrypted compaction state (gAAAA...) unchanged on both normal /responses continuation and compact requests

This replaces the earlier too-broad encrypted-state rewrite: valid provider compact state is authoritative continuity and must stay opaque, not become [compact state] [unverified compact state omitted].

Live failure and follow-up

The original invalid-encrypted-content failures were caused by replaying local/plaintext compact fallback state as a provider compaction item. A later live post-compact hang showed the opposite hazard too: replacing valid provider encrypted compact state broke continuation context. This PR now keeps the two cases separate.

Testing

  • uv run pytest tests/unit/test_openai_requests.py::test_compact_rewrites_plaintext_compaction_replay_without_encrypted_content tests/unit/test_openai_requests.py::test_compact_preserves_provider_encrypted_compaction_replay tests/unit/test_openai_requests.py::test_responses_preserves_provider_encrypted_compaction_replay tests/unit/test_openai_requests.py::test_compact_strips_poisoned_local_compact_fallback_items tests/unit/test_openai_requests.py::test_responses_strips_poisoned_local_compact_fallback_items -> 5 passed
  • uv run pytest tests/unit/test_openai_requests.py -> 172 passed
  • PR scope gate: git diff --name-only origin/main...HEAD contains only app/core/openai/requests.py and tests/unit/test_openai_requests.py

@Komzpa
Komzpa force-pushed the fix/sanitize-encrypted-compaction-replays branch from cabe4ad to a4d37ff Compare August 13, 2026 13:08

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: cabe4ad369

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread app/core/openai/requests.py Outdated
Comment thread app/core/openai/requests.py Outdated
Comment thread tests/unit/test_openai_requests.py
Comment thread app/core/openai/requests.py Outdated
Comment thread app/core/openai/requests.py Outdated
@Komzpa
Komzpa force-pushed the fix/sanitize-encrypted-compaction-replays branch from a4d37ff to a06a097 Compare August 14, 2026 03:52
@Komzpa Komzpa changed the title fix(compact): sanitize encrypted compaction replays fix(compact): sanitize plaintext compaction replays Aug 14, 2026
@Komzpa Komzpa added the 🤖 codex: needs work [@codex review] raised an issue label Aug 14, 2026
@Komzpa
Komzpa force-pushed the fix/sanitize-encrypted-compaction-replays branch from a06a097 to cb7c9ae Compare August 14, 2026 03:54
@Komzpa Komzpa removed the 🤖 codex: needs work [@codex review] raised an issue label Aug 14, 2026
@Komzpa
Komzpa force-pushed the fix/sanitize-encrypted-compaction-replays branch from cb7c9ae to 75e7997 Compare August 14, 2026 04:07
@Komzpa

Komzpa commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 75e7997236

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread app/core/openai/requests.py Outdated
Comment thread app/core/openai/requests.py Outdated
@Komzpa Komzpa added 🤖 codex: needs work [@codex review] raised an issue and removed 🤖 codex: needs work [@codex review] raised an issue labels Aug 14, 2026
@Komzpa
Komzpa force-pushed the fix/sanitize-encrypted-compaction-replays branch from 5a2b9d0 to 0ff7778 Compare August 14, 2026 18:52
@Komzpa

Komzpa commented Aug 14, 2026

Copy link
Copy Markdown
Collaborator Author

@codex review

@chatgpt-codex-connector chatgpt-codex-connector Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 0ff7778a47

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

  • Open a pull request for review
  • Mark a draft as ready
  • Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

Comment thread app/modules/proxy/replay_safety.py
Comment thread app/modules/proxy/replay_safety.py
Comment thread app/modules/proxy/_service/websocket/helpers.py
Comment on lines +3028 to +3029
request_state.preferred_account_id = None
request_state.excluded_account_ids.add(session.account.id)

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Badge Keep recovery-lane reconnects on their current account

When a request is already running in an account-neutral recovery lane and its current upstream remains silent, this branch now clears the preferred owner and explicitly excludes the lane's current account, allowing the same recovered task to move again. That contradicts openspec/changes/classify-required-http-bridge-owner-unavailable/specs/responses-api-compat/spec.md:147-160, which requires reconnects for an established recovery lane to retain its current account and typed ownership provenance; otherwise a second ambiguous resend can lose the recovered turn state or duplicate work on another account.

AGENTS.md reference: AGENTS.md:L105-L110

Useful? React with 👍 / 👎.

@Komzpa Komzpa added the 🤖 codex: needs work [@codex review] raised an issue label Aug 14, 2026
@Komzpa
Komzpa force-pushed the fix/sanitize-encrypted-compaction-replays branch from 381247d to db7ce90 Compare August 14, 2026 21:43
@Komzpa Komzpa removed the 🤖 codex: needs work [@codex review] raised an issue label Aug 14, 2026
@Soju06

Soju06 commented Aug 16, 2026

Copy link
Copy Markdown
Owner

Thanks — the compaction-replay core of this PR looks right now: preserving any string encrypted_content verbatim (no ciphertext-shape heuristics) and rewriting only plaintext summary/text/content shapes into canonical output_text assistant items resolves both live failure modes, and the iteration through the review rounds (opaque envelopes, canonical content, owner-bound fail-closed, server-executed tool-search rejection) reads well. A few things block merge as-is:

  1. CI is red because of an orphaned test. tests/integration/test_http_responses_bridge.py:6992 (test_retry_account_neutral_precreated_request_switches_from_silent_account, added in c5b5f9d) asserts that the silent-account retry clears preferred_account_id and excludes the lane's account — but db7ce90 reverted exactly that behavior in request_submit.py (correctly, per the recovery-lane review comment). The test now fails deterministically (excluded_account_ids == set()). Please delete it, or rewrite it to assert the current invariant (lane keeps its account, require_preferred_account=True), which would actually lock in the fix from that round.

  2. Empty-string encrypted_content edge. _plaintext_compaction_replay_replacement (app/core/openai/requests.py:943) preserves any str encrypted_content, including "". An empty encrypted payload will still be replayed upstream and hit the invalid-encrypted-content failure this PR targets. Suggest isinstance(encrypted_content, str) and encrypted_content, matching the non-empty check the old poison-skip path used.

  3. Dead code in replay_safety. _compaction_item_is_self_contained (replay_safety.py:671) is constant False, which makes the new compaction allowlist entries and the continue at line 1055 unreachable. The fail-closed outcome is what we want, but stating it directly (reject item_type == "compaction") would be clearer than a predicate that implies some shapes could qualify.

  4. Scope and description drift. The PR body still claims the diff touches only requests.py + its unit tests and cites test names that no longer exist, but the branch now also carries a full tool_search replay feature (previous-response trimming on both bridges, account-neutral classification) and edits an archived openspec change to document it. Could you update the body to match, and move the tool-search spec delta out of openspec/changes/archive/2026-08-13-... into a live change folder (as you already did for the compaction contract)? Splitting the tool-search work into its own PR would also make both easier to land, but at minimum the description and archive edit need fixing.

Happy to re-review once CI is green.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants